Add ResolveGroupingAnalytics analyzer rule #5749

mingmwang · 2023-03-27T05:42:22Z

Which issue does this PR close?

Closes #5672
Closes #5649
Closes #5647.

Some TPC-DS(q27, q36, q70, q86)queries will be impacted by this PR also. Those queries include the GROUPING() functions
and the actually physical plans were not able to evaluate before this PR.

Rationale for this change

What changes are included in this PR?

Add new ResolveGroupingAnalytics rule, move the enumerate_grouping_sets logic to this rule
Add two internal hidden columns when necessary

// Internal column used to represent the grouping_id, used by the grouping functions.
// It is "spark_grouping_id" in Spark
const INTERNAL_GROUPING_ID: &str = "grouping_id";
// Internal column used to represent different grouping sets when there are duplicated grouping sets
const INTERNAL_GROUPING_SET_ID: &str = "grouping_set_id";

Support GROUPING() and GROUPING_ID() functions.

Are these changes tested?

Are there any user-facing changes?

mingmwang · 2023-03-27T06:30:46Z

@jackwener @yukkit @alamb

alamb · 2023-04-04T17:19:38Z

I am not an expert in GROUPING sets / etc. I wonder if someone else has the time to review this PR?

mingmwang · 2023-04-06T12:39:41Z

@yjshen Could you please help to review this PR ?

jackwener · 2023-04-10T08:45:17Z

It looks like no one has reviewed this PR, and I'm not an expert about GROUP.
so I'm going to review this PR while looking up information. It need some time.

jackwener · 2023-04-16T13:51:28Z

spark: https://issues.apache.org/jira/browse/SPARK-17868 apache/spark#15484

jackwener

Thank you @mingmwang , It's a excellent job!

jackwener · 2023-04-16T04:14:38Z

datafusion/expr/src/aggregate_function.rs

+        match self {
+            AggregateFunction::GroupingId => {
+                write!(f, "GROUPING_ID")
+            }


look like we don't need it

jackwener · 2023-04-16T13:45:09Z

datafusion/core/tests/sql/group_by.rs

@@ -110,6 +110,28 @@ async fn csv_query_group_by_boolean() -> Result<()> {
    Ok(())
 }

+#[tokio::test]


A future ticket: add some test in slt

jackwener · 2023-04-16T13:46:05Z

datafusion/sql/tests/integration_test.rs

@@ -2856,8 +2856,8 @@ fn aggregate_with_rollup() {
    let sql =
        "SELECT id, state, age, COUNT(*) FROM person GROUP BY id, ROLLUP (state, age)";
    let expected = "Projection: person.id, person.state, person.age, COUNT(UInt8(1))\
-    \n  Aggregate: groupBy=[[GROUPING SETS ((person.id), (person.id, person.state), (person.id, person.state, person.age))]], aggr=[[COUNT(UInt8(1))]]\
-    \n    TableScan: person";
+        \n  Aggregate: groupBy=[[person.id, ROLLUP (person.state, person.age)]], aggr=[[COUNT(UInt8(1))]]\


jackwener · 2023-04-16T13:49:21Z

datafusion/physical-expr/src/expressions/column.rs

@@ -202,6 +202,77 @@ impl PartialEq<dyn Any> for UnKnownColumn {
    }
 }

+#[derive(Debug, Hash, PartialEq, Eq, Clone)]


jackwener · 2023-04-16T14:06:02Z

datafusion/core/src/physical_plan/aggregates/mod.rs

+                let mut group_data = group
+                    .iter()
+                    .enumerate()
+                    .map(|(idx, is_null)| {
+                        if *is_null {
+                            null_exprs_value[idx].clone()
+                        } else {
+                            exprs_value[idx].clone()
+                        }
+                    })
+                    .collect::<Vec<_>>();


we can extract a function named get_group_data.

jackwener · 2023-04-16T14:06:23Z

datafusion/core/src/physical_plan/aggregates/mod.rs

+                group
+                    .iter()
+                    .enumerate()
+                    .map(|(idx, is_null)| {
+                        if *is_null {
+                            null_exprs_value[idx].clone()
+                        } else {
+                            exprs_value[idx].clone()
+                        }
+                    })
+                    .collect::<Vec<_>>()


use function get_group_data

jackwener · 2023-04-16T14:09:53Z

datafusion/core/src/physical_plan/aggregates/mod.rs

+    /// Hidden grouping set expr in the grouping set
+    hidden_grouping_set_expr: Vec<(Arc<dyn PhysicalExpr>, String)>,
+    /// Distinct result expr for the grouping set, used to generate output schema
+    result_expr: Vec<(Arc<dyn PhysicalExpr>, String)>,


I have a question.

Is result_expr a part of grouping_set_expr?
If yes, maybe we can use a index vec to point grouping_set_expr

jackwener · 2023-04-16T14:27:55Z

datafusion/optimizer/src/push_down_filter.rs

+                    if !expr.contains_hidden_columns()
+                        && cols.iter().all(|c| group_expr_columns.contains(c))
+                    {


hiddent columns may influent optimizer rule, it may cause some bug.
We should add more test to cover it in the following PR.

jackwener · 2023-04-16T14:30:20Z

datafusion/expr/src/expr.rs

@@ -1364,6 +1423,8 @@ fn create_name(e: &Expr) -> Result<String> {
            "Create name does not support qualified wildcard".to_string(),
        )),
        Expr::Placeholder { id, .. } => Ok((*id).to_string()),
+        Expr::HiddenColumn(_, c) => Ok(format!("#{}", c)),
+        Expr::HiddenExpr(first, _) => Ok(format!("#{}", first)),


Should HiddenExpe be Ok(format!("{}", first))? look like # is wrong.

jackwener · 2023-04-16T14:35:38Z

datafusion/optimizer/src/analyzer/resolve_grouping_analytics.rs

+
+pub struct ResolveGroupingAnalytics;


We can add some comment to explain this rule resolve what problem.

alamb · 2023-05-03T20:02:59Z

What is the status of this PR? It has accumulated non trivial conflicts. Shall we mark it as a draft ?

mingmwang · 2023-05-04T02:55:13Z

What is the status of this PR? It has accumulated non trivial conflicts. Shall we mark it as a draft ?

Yes, please mark it as a draft.

alamb · 2024-04-08T21:05:49Z

Since this has been open for more than a year, closing it down. Feel free to reopen if/when you keep working on it.

mingmwang added 6 commits March 21, 2023 15:50

add analyzer rule replace_grouping_func

8e87787

merge with latest

1acc425

add VirtualColumn

6a9fbf3

fix common_subexpr_eliminate

1cee4f4

merge with upstream

3c4d8b0

Add ResolveGroupingAnalytics analyzer rule

b3c41a1

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner labels Mar 27, 2023

merge with upstream

2b183f4

mingmwang added 3 commits March 27, 2023 14:38

fix failed UT, make the sort result stable

4e065c9

merge with upstream, resolve conflicts

7f68b32

minor change

f6059e8

mingmwang mentioned this pull request Mar 29, 2023

Improve the performance of COUNT DISTINCT queries for high cardinality groups #5547

Closed

jackwener self-requested a review April 5, 2023 11:08

jackwener reviewed Apr 16, 2023

View reviewed changes

jackwener marked this pull request as draft May 4, 2023 11:10

alamb closed this Apr 8, 2024

eejbyfeldt mentioned this pull request Sep 20, 2024

Support Grouping functions with Group By CUBE/ROLLUP/GROUPING SETS #5647

Closed

This was referenced Sep 21, 2024

fix: Correct results for grouping sets when columns contain nulls #12571

Merged

Implement GROUPING aggregate function (following Postgres behavior.) #12565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ResolveGroupingAnalytics analyzer rule #5749

Add ResolveGroupingAnalytics analyzer rule #5749

mingmwang commented Mar 27, 2023 •

edited by alamb

Loading

mingmwang commented Mar 27, 2023

alamb commented Apr 4, 2023

mingmwang commented Apr 6, 2023

jackwener commented Apr 10, 2023 •

edited

Loading

jackwener commented Apr 16, 2023

jackwener left a comment

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

jackwener Apr 16, 2023

alamb commented May 3, 2023

mingmwang commented May 4, 2023

alamb commented Apr 8, 2024

Add ResolveGroupingAnalytics analyzer rule #5749

Add ResolveGroupingAnalytics analyzer rule #5749

Conversation

mingmwang commented Mar 27, 2023 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mingmwang commented Mar 27, 2023

alamb commented Apr 4, 2023

mingmwang commented Apr 6, 2023

jackwener commented Apr 10, 2023 • edited Loading

jackwener commented Apr 16, 2023

jackwener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 3, 2023

mingmwang commented May 4, 2023

alamb commented Apr 8, 2024

mingmwang commented Mar 27, 2023 •

edited by alamb

Loading

jackwener commented Apr 10, 2023 •

edited

Loading